src/main/java/de/tum/cit/aet/artemis/core/config/PublicResourcesConfiguration.java (1)

Line range hint 36-62: Summary: Caching implementation for static resources looks good

The changes to PublicResourcesConfiguration successfully implement caching for course icons, profile pictures, and drag-and-drop quiz pictures. This aligns with the PR objective of improving performance by reducing the number of times these images are loaded from the server.

Key points:

1. The fileUploadPath is correctly injected using @Value.
2. Resource handlers are added for each type of static resource.
3. Caching is applied consistently using defaultCacheControl.

Suggestions for improvement:

1. Address the TODO comment by extracting path constants to avoid duplication.
2. Consider minor formatting adjustments for consistency.

Overall, these changes should contribute to improved performance as intended. Remember to update the relevant tests and documentation to reflect these changes.

src/main/webapp/app/overview/course-conversations/layout/conversation-messages/conversation-messages.component.ts (2)

Line range hint 139-149: LGTM! Consider calling subscribeToMetis() in ngOnInit().

The new subscribeToMetis() method is well-implemented and follows Angular best practices. It correctly subscribes to the necessary observables and updates the component's state.

To ensure this subscription is set up when the component initializes, consider adding a call to subscribeToMetis() in the ngOnInit() method.

You can add the following line to the ngOnInit() method:

this.subscribeToMetis();

---

Line range hint 172-172: LGTM! Consider using arrow function syntax.

The modification to reverse the order of posts before assignment is a good change that aligns with the PR objectives. This will likely improve the performance of the component when rendering posts.

To maintain consistency with the coding guidelines, consider using arrow function syntax for this method.

You can refactor the method as follows:

setPosts = (posts: Post[]): void => {
    if (this.content) {
        this.previousScrollDistanceFromTop = this.content.nativeElement.scrollHeight - this.content.nativeElement.scrollTop;
    }
    this.posts = posts.slice().reverse();
};

src/main/java/de/tum/cit/aet/artemis/core/web/FileResource.java (2)

Line range hint 1-665: Consider reviewing and aligning caching across all relevant endpoints.

While the changes in the responseEntityForFilePath method are excellent, it might be beneficial to ensure consistency in caching across all relevant endpoints in this file.

Review other methods that serve files, such as getCourseIcon, getProfilePicture, and others, to ensure they also implement appropriate caching strategies. This would provide a consistent performance improvement across the application.

For example, you might want to update the getCourseIcon method:

@GetMapping("files/course/icons/{courseId}/*")
@EnforceAtLeastStudent
public ResponseEntity<byte[]> getCourseIcon(@PathVariable Long courseId) {
    log.debug("REST request to get icon for course : {}", courseId);
    Course course = courseRepository.findByIdElseThrow(courseId);
-   return responseEntityForFilePath(getActualPathFromPublicPathString(course.getCourseIcon()));
+   return responseEntityForFilePath(getActualPathFromPublicPathString(course.getCourseIcon())).cacheControl(CacheControl.maxAge(FILE_CACHE_DAYS, TimeUnit.DAYS));
}

Apply similar changes to other relevant methods to ensure consistent caching behavior across the application.

---

Caching not properly implemented for course icons and profile pictures.

The current implementation of getCourseIcon and getProfilePicture does not enable caching by setting the cache parameter to true in the responseEntityForFilePath method. This means caching headers are not applied, contrary to the PR objectives.

Recommendation:

• Update the getCourseIcon and getProfilePicture methods to call responseEntityForFilePath with the cache parameter set to true to ensure proper caching.

🔗 Analysis chain

Line range hint 1-665: Verify caching implementation for course icons and profile pictures.

The PR objectives mention caching course icons and profile pictures. While the changes in the responseEntityForFilePath method provide a good foundation for caching, we should verify that this implementation effectively caches these specific resources.

To ensure that the caching is correctly applied to course icons and profile pictures, we can run the following script:

This script will help us verify that the caching implementation is correctly applied to the course icons and profile pictures, as specified in the PR objectives.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify caching implementation for course icons and profile pictures

# Test: Check if caching is applied to course icon endpoints
echo "Checking course icon endpoints:"
rg --type java -C 3 "files/course/icons.*ResponseEntity"

# Test: Check if caching is applied to profile picture endpoints
echo "Checking profile picture endpoints:"
rg --type java -C 3 "files/user/profile-pictures.*ResponseEntity"

# Test: Verify that responseEntityForFilePath is used in these methods
echo "Verifying usage of responseEntityForFilePath:"
rg --type java -C 3 "responseEntityForFilePath.*course.getCourseIcon"
rg --type java -C 3 "responseEntityForFilePath.*user.getImageUrl"

Length of output: 2178

Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE

src/test/java/de/tum/cit/aet/artemis/core/util/CourseTestService.java (6)

Line range hint 487-488: Missing @Test Annotation on Test Methods

The test methods are missing the @Test annotation from JUnit 5, which is necessary for the test runner to recognize and execute them.

Apply this diff to add the @Test annotation to each test method:

+import org.junit.jupiter.api.Test;

 // Test
+@Test
 public void testCreateCourseWithPermission() throws Exception {
     // test implementation
 }

 // Test
+@Test
 public void testCreateCourseWithSameShortName() throws Exception {
     // test implementation
 }

 // Test
+@Test
 private void testCreateCourseWithNegativeValue(Course course) throws Exception {
     // test implementation
 }

 // Test
+@Test
 public void testCreateCourseWithNegativeMaxComplainNumber() throws Exception {
     // test implementation
 }
 
 // Continue adding @Test annotations to the remaining test methods...

Also applies to: 495-496, 503-504, 511-512, 519-520, 527-528, 537-538, 545-546, 553-554

---

Line range hint 675-683: Avoid Using Magic Numbers in Test Setup

In the setup method, NUMBER_OF_STUDENTS, NUMBER_OF_TUTORS, NUMBER_OF_EDITORS, and NUMBER_OF_INSTRUCTORS are hardcoded values. It's better to use constants or configuration parameters to improve readability and maintainability.

Consider defining these numbers as configurable constants or parameters:

 public class CourseTestService {

     private static final int NUMBER_OF_STUDENTS = 8;
     private static final int NUMBER_OF_TUTORS = 5;
     private static final int NUMBER_OF_EDITORS = 1;
     private static final int NUMBER_OF_INSTRUCTORS = 1;

     public void setup(String userPrefix, MockDelegate mockDelegate) {
-        userUtilService.addUsers(userPrefix, 8, 5, 1, 1);
+        userUtilService.addUsers(userPrefix, NUMBER_OF_STUDENTS, NUMBER_OF_TUTORS, NUMBER_OF_EDITORS, NUMBER_OF_INSTRUCTORS);
         // rest of the setup method...
     }

---

Line range hint 1824-1834: Handle Potential NullPointerException

In the method testGetCourseForDashboard, the variable courses.getFirst() is used without checking if the list courses is not empty. This could lead to a NullPointerException if courses is empty.

Consider adding a check to ensure that courses is not empty before calling getFirst():

 public void testGetCourseForDashboard(boolean userRefresh) throws Exception {
     List<Course> courses = courseUtilService.createCoursesWithExercisesAndLecturesAndLectureUnitsAndCompetencies(userPrefix, true, false, NUMBER_OF_TUTORS);
+    if (courses.isEmpty()) {
+        throw new IllegalStateException("Course list is empty.");
+    }
     CourseForDashboardDTO receivedCourseForDashboard = request.get("/api/courses/" + courses.getFirst().getId() + "/for-dashboard?refresh=" + userRefresh, HttpStatus.OK,
             CourseForDashboardDTO.class);
     Course receivedCourse = receivedCourseForDashboard.course();
     // rest of the method...
 }

---

Line range hint 2446-2460: Possible Redundant Null Check

In the method testArchiveCourseWithTestModelingAndFileUploadExercises, after the archiving process, the code retrieves the updated course and checks assertThat(updatedCourse.getCourseArchivePath()).isNotEmpty();. Since getCourseArchivePath() returns a String, which cannot be null when isNotEmpty() is called, the null check might be redundant.

Ensure that the null check is necessary; otherwise, consider removing it to clean up the code.

---

Line range hint 799-806: Unused Import Statements

There are several unused import statements at the top of the file. Unused imports can clutter the code and may lead to confusion.

Consider removing unused imports to clean up the code.

For example:

-import org.mockito.MockedStatic;

---

Line range hint 2540-2546: Consistent Use of Assertions

In testDownloadCourseArchiveAsInstructor_not_found, the assertion checks for assertThat(downloadedArchive).isNull();. In other test methods, similar checks use assertThat(response).as("message").isNull();. For consistency, consider adding a description to the assertion.

-assertThat(downloadedArchive).isNull();
+assertThat(downloadedArchive).as("No archive should be downloaded as it does not exist").isNull();

Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE